fix(extensions,presets): surface clean error on malformed download URL#3577
Merged
mnriem merged 2 commits intoJul 21, 2026
Merged
Conversation
`ExtensionCatalog.download_extension` and `PresetCatalog.download_pack` read `download_url` from catalog payload data and pass it to `urlparse(...).hostname` during the HTTPS validation. A malformed authority (e.g. an unterminated IPv6 bracket like `https://[::1`) makes urlparse/hostname raise a raw `ValueError`, which escapes past the command handlers — they only catch `ExtensionError` / `PresetError` — and surfaces as an uncaught traceback. Guard the parse in a try/except and re-raise as the domain error so the CLI reports a clean "download URL is malformed" message. Mirrors the same fix in catalogs (github#3435) and workflows/catalog.py (github#3484). Adds regression coverage for both catalogs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Converts malformed extension and preset download URLs into domain-specific errors.
Changes:
- Guards URL parsing and hostname access.
- Adds regression coverage for malformed IPv6 authorities.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/extensions/__init__.py |
Raises ExtensionError for malformed URLs. |
src/specify_cli/presets/__init__.py |
Raises PresetError for malformed URLs. |
tests/test_extensions.py |
Tests malformed extension URLs. |
tests/test_presets.py |
Tests malformed preset URLs. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
mnriem
requested changes
Jul 17, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback
Copilot review on github#3577 flagged that the malformed-URL fix stopped short: `download_pack` now raises a clean `PresetError`, but the `preset_add` handler rendered `{e}` unescaped. A catalog `download_url` like `https://[not-an-ip]/x` is embedded verbatim in the message, so Rich interprets `[not-an-ip]` as a markup tag and can raise a style/markup exception while rendering the error — the CLI still crashes instead of exiting cleanly. Escape `str(e)` in the preset command handlers, matching the extension handler at `extensions/_commands.py:657`, and hoist the `rich.markup` import to module scope (dropping the two inline imports). Adds CLI-level regression tests: a bracketed-host `download_url` exits cleanly, and the compatibility/validation/error handlers escape markup-bearing messages. Both tests fail on the pre-fix handler (test-the-test verified). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@ The malformed-URL fix stopped short: Changes:
Full |
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
src/specify_cli/presets/init.py:2728
download_pack()is also called byspecify init --preset(commands/init.py:588). That caller catchesPresetErrorand passes it to_print_cli_warning(), which interpolatesstr(exc)into Rich markup without escaping (specify_cli/__init__.py:355). Forhttps://[not-an-ip]/x, this new message therefore triggers a Rich style-parsing error instead of reporting the malformed URL (the outer init handler may then report the renderer error). Please escape dynamic warning details there as well and cover the init path, or avoid embedding markup-sensitive catalog data in this exception.
raise PresetError(
f"Preset download URL is malformed: {download_url}"
) from None
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Medium
mnriem
self-requested a review
July 21, 2026 13:33
mnriem
approved these changes
Jul 21, 2026
Collaborator
|
Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ExtensionCatalog.download_extensionandPresetCatalog.download_packread adownload_urlfrom catalog payload data and pass it throughurlparse(...).hostnameduring their HTTPS validation. A malformed authority — e.g. an unterminated IPv6 bracket likehttps://[::1, orhttps://[not-an-ip]/x— makesurlparse/.hostnameraise a rawValueError.That
ValueErrorescapes the command handlers, which only catchExtensionError/PresetError, so instead of a clean CLI message the user gets an uncaught traceback.This wraps the parse in a
try/except ValueErrorand re-raises as the module's domain error (ExtensionError/PresetError) with a clear "download URL is malformed" message.Bug class
Same shape as the already-fixed unguarded-
urlparseleaks in:workflows/catalog.py(fix(workflows): raise catalog error, not raw ValueError, on a malformed catalog URL #3484)This closes the two remaining sibling code paths (extensions + presets) that had the same latent crash.
Changes
src/specify_cli/extensions/__init__.py— guard thedownload_urlparse indownload_extension.src/specify_cli/presets/__init__.py— guard thedownload_urlparse indownload_pack.tests/test_extensions.py— regression test asserting a malformed URL raisesExtensionError(notValueError).tests/test_presets.py— matching regression test fordownload_pack.Testing
ValueError) and pass with the guard ("test-the-test" verified).tests/test_extensions.pyandtests/test_presets.pypass (717 passed, 5 skipped).🤖 Generated with Claude Code